snc: generate ca cert for snc if not provided by user#490
Conversation
anjannath
changed the title
|
may it is worthwhile if the cert is generated by mapt ...to add it as an output and write it to a file...in case the user woould need it to set something on his env. Otherwise this LGTM and if we want to write we can do it on a different PR |
adrianriobo
left a comment
adrianriobo
left a comment
There was a problem hiding this comment.
LGTM
I still see WIP so up to you check comments or just merge
yes, currently it writes to the working dir, i'll update to write it in the outputs dir and handle it as a pulumi output |
|
Ohh I see...but yeah may write it as other outputs so we can ensure all goes to same path based on the output path param |
there are two scenarios, one is where the user provides the cert in that case we don't want to again write/copy it to the output path, but in the situation where |
anjannath
changed the title
| return fmt.Errorf("no instances matching criteria") | ||
| } | ||
| if len(args.CaCertFile) == 0 { | ||
| args.CaCertFile = util.GenAdminKubeconfigSignerCert() |
There was a problem hiding this comment.
For the case you mention... I would suggets to remove this check here...
Then move the check to the deploy function....you should check cacert on the request...if is empty..you create it (if you create inside the if..you also add it as export)
Then on the manageOutputs...you can check for results[keyCaCert] if ok....if ok means it was empty so you created it....then your write it...otherwise you do not write it cause it was set by the user.
Does it make sense to you>
There was a problem hiding this comment.
Yes, thanks! let me try this..
anjannath
force-pushed
the
ocp-cert-gen
branch
2 times, most recently
from
e8421f9 to
9078119
Compare
anjannath
changed the title
| if err != nil { | ||
| return err | ||
| } | ||
| ctx.Export(fmt.Sprintf("%s-%s", *r.prefix, outputCaCert), |
There was a problem hiding this comment.
What is exported here...the path or the content?
| } | ||
|
|
||
| caCertPath, ok := stackResult.Outputs[caCertKey].Value.(string) | ||
| if ok { |
There was a problem hiding this comment.
I would suggest ..from previous comment to export the content...then here... I would move the stackResult.Outputs...check before setting the results .....
if the check is ok...you neeed to add the caCertKey := fmt.Sprintf("%s-%s", *prefix, outputCaCert) to results...as the hostIPKey...then the write will take care of write the content on the file on the expected output...
There was a problem hiding this comment.
this also needed changes to the r.userData function as that expects r.caCertFile to be a file path, rather than the content
anjannath
force-pushed
the
ocp-cert-gen
branch
2 times, most recently
from
c9d02b8 to
d975a2f
Compare
anjannath
changed the title
anjannath
force-pushed
the
ocp-cert-gen
branch
3 times, most recently
from
f3fc8e7 to
cf6a689
Compare
|
This LGTM so feel free to merge when you feel it is finished ^^ @anjannath |
ack, thanks.. but in crc-org/snc#1088 we are removing the need for the user to provide the CA files, so the |
anjannath
changed the title
|
🤔 are you moving the logic inside the image, and removing any option to set the CA externally?.... I thought that would be a really nice feature to have... as it may happen on some orgs..they need any cluster with certs would use their own CA |
they'll still be able to do it later as day-2 operations on the cluster.. https://access.redhat.com/solutions/6054981 so there are can be two scenarios..
in the first case its actually better to handle this in the image, since the user won't have to manually generate it and its all handled for him in case of the user wanting to use an existing CA (self-signed or existing CA), they'll additionally have to also provide a client certificate so that we can verify after rotating the CA if we still have access to the cluster... so in the self sufficient service, at first we add the new CA as an additional CA for client auth, then once we are sure that with the new CA we are able to access the cluster, only then we invalidate the old CA and replace the if both the CA and client certificate is provided externally and we also want to handle the case where these needs to be generated inside the image, it increases the complexity of the service inside the image.. maybe if there's demand for this feature, we can add it later.. (will also need changes the service in snc) |
|
I see.... fair enough.... although now with scenario 1 ensure we get the CA from the machine (as we do with Kubeconfig) as potentially they may need st that CA somewhere while using the cluster .... we get and export it (CA) as the kubeconfig |
|
Also if possible link the issue / pr from snc here |
in the self sufficient bundle the CA cert to be used for client authentication is generated on the VM itself after start, users don't have to pass it manually to `mapt` this was done in snc by the following PR: crc-org/snc@09c643d
this runs `make fmt` and fixes the formatting issue in the azure/data/images.go file
anjannath
changed the title
yes, the CA is embedded in the |
the ocp-cluster-ca.service re-creates the kubeconfig file and invalidates the old kubeconfig, we need to make sure to copy the kubeconfig after that service has finished to be able to access the cluster with it
|
@adrianriobo Tested the latest iteration with 4.19 snc bundles and this is now ready for another review, I've updated it as per the comments, and added a new commit to copy the kubeconfig file for output after the |
github-project-automation
Bot
moved this from Work In Progress
to Done
in Project planning: crc
2 participants
for the openshift-snc service user needs to supply a self signed cert for the admin kubeconfig, this commit adds code to generate this cert when the user has not provided one during create using the flag '--ca-cert-file'
related to crc-org/snc#1088